[DirectX] Do not flatten GEP chains for unsupported types #150484
Conversation
|
@llvm/pr-subscribers-backend-directx Author: Deric C. (Icohedron) ChangesFixes #150463 by not processing GEPs for unsupported types such as structs in the DXILFlattenArrays pass. Full diff: https://github.com/llvm/llvm-project/pull/150484.diff 1 Files Affected:
diff --git a/llvm/lib/Target/DirectX/DXILFlattenArrays.cpp b/llvm/lib/Target/DirectX/DXILFlattenArrays.cpp
index f0e2e786dfaf4..b7f81c4b3e357 100644
--- a/llvm/lib/Target/DirectX/DXILFlattenArrays.cpp
+++ b/llvm/lib/Target/DirectX/DXILFlattenArrays.cpp
@@ -263,8 +263,13 @@ bool DXILFlattenArraysVisitor::visitGetElementPtrInst(GetElementPtrInst &GEP) {
// merge the byte offsets. Otherwise, this GEP is itself the root of a GEP
// chain and we need to deterine the root array type
if (auto *PtrOpGEP = dyn_cast<GEPOperator>(PtrOperand)) {
- assert(GEPChainInfoMap.contains(PtrOpGEP) &&
- "Expected parent GEP to be visited before this GEP");
+
+ // If the parent GEP was not processed, then we do not want to process its
+ // descendants. This can happen if the GEP chain is for an unsupported type
+ // such as structs -- we do not flatten structs.
+ if (!GEPChainInfoMap.contains(PtrOpGEP))
+ return false;
+
GEPInfo &PGEPInfo = GEPChainInfoMap[PtrOpGEP];
Info.RootFlattenedArrayType = PGEPInfo.RootFlattenedArrayType;
Info.RootPointerOperand = PGEPInfo.RootPointerOperand;
|
There was a problem hiding this comment.
Looks to me like line 298 is the way out for parent GEPs that have a struct type or otherwise unsupported type.
Might be worth updating the comment with something like, "we also don't need to do any flattening for children GEPs if the type is unsupported, e.g., a struct"
But that's a nit / verifying my understanding is correct. Code / test LGTM
| assert(GEPChainInfoMap.contains(PtrOpGEP) && | ||
| "Expected parent GEP to be visited before this GEP"); | ||
|
|
||
| // If the parent GEP was not processed, then we do not want to process its |
There was a problem hiding this comment.
The comment makes sense as does the fix, Was wondering should we have something more explict about type checking for structs and bailing?
There was a problem hiding this comment.
Agreed, I think the comment on 296 is a good spot to indicate that
There was a problem hiding this comment.
I could throw an error when there is an unsupported type, but would the DXILFlattenArrays pass really be the place to throw such errors?
Line 296 and below already has
// If the root type is not an array, we don't need to do any flattening
if (!isa<ArrayType>(RootTy))
return false;to indicate that only arrays are applicable to this transformation.
The name of the pass "DXIL Flatten Arrays" to me also implies it only applies to arrays.
There was a problem hiding this comment.
This can be addressed in a follow-up PR if needed.
Fixes llvm#150463 by not processing GEPs for unsupported types such as structs in the DXILFlattenArrays pass.
Fixes #150463 by not processing GEPs for unsupported types such as structs in the DXILFlattenArrays pass.